Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to fix flaky TestGetRoundTripper* tests #4738

Merged
merged 6 commits into from
Sep 9, 2023

Conversation

albertteoh
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Introduces an atomic boolean outside of the HTTP test handler and only set to true if the expected request is received.
  • This should prevent cases where unrelated processes are hitting the same test URL.

How was this change tested?

  • Ran make test to pass.
  • Ran go test -tags=memory_storage_integration -count 10 ./... and confirmed metricsstore tests are passing.
  • Note that a number of unrelated tests were failing with the -count 10 flag set, not as a result of the changes from this PR, namely:
--- FAIL: TestFactory (0.00s)
    --- FAIL: TestFactory/#00 (0.00s)
panic: Reuse of exported var name: test_1694287567920905000_counter_x
 [recovered]
	panic: Reuse of exported var name: test_1694287567920905000_counter_x

and

--- FAIL: TestPublishOpts (0.00s)
    builder_test.go:308:
        	Error Trace:	/Users/albertteoh/go/src/github.com/albertteoh/jaeger/cmd/agent/app/builder_test.go:308
        	Error:      	Received unexpected error:
        	            	cannot create processors: cannot create Thrift Processor: cannot create UDP Server: cannot create UDPServerTransport: listen udp :5775: bind: address already in use
        	Test:       	TestPublishOpts

Checklist

Albert Teoh added 4 commits September 10, 2023 05:04
Signed-off-by: Albert Teoh <albert@packsmith.io>
Signed-off-by: Albert Teoh <albert@packsmith.io>
Signed-off-by: Albert Teoh <albert@packsmith.io>
Signed-off-by: Albert Teoh <albert@packsmith.io>
@albertteoh albertteoh requested a review from a team as a code owner September 9, 2023 19:38
@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Patch has no changes to coverable lines.

📢 Thoughts on this report? Let us know!.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using boolean in the assert is not the best output when the assert fails. Using the actual string produces much more intuitive output in case of test failure.

@@ -537,10 +538,14 @@ func TestGetRoundTripperTLSConfig(t *testing.T) {
}, logger)
require.NoError(t, err)

var authReceived atomic.Bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var authReceived atomic.Bool
var authReceived atomic.Pointer[string]

Comment on lines 546 to 548
if r.Header.Get("Authorization") == "Bearer foo" {
authReceived.Store(true)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if r.Header.Get("Authorization") == "Bearer foo" {
authReceived.Store(true)
}
h := r.Header.Get("Authorization")
authReceived.Store(&h)

assert.Equal(t, http.StatusOK, resp.StatusCode)
assert.True(t, authReceived.Load())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.True(t, authReceived.Load())
assert.Equal(t, "Bearer foo", *authReceived.Load())

if err != nil {
return err
}
require.NoError(t, err)

_, err = fmt.Fprintln(w, string(bytes))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_, err = fmt.Fprintln(w, string(bytes))
_, err = w.Write(bytes)

Signed-off-by: Albert Teoh <albert@packsmith.io>
@albertteoh
Copy link
Contributor Author

Using boolean in the assert is not the best output when the assert fails. Using the actual string produces much more intuitive output in case of test failure.

Good suggestion, addressed in: 31bdc02

yurishkuro
yurishkuro previously approved these changes Sep 9, 2023
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro enabled auto-merge (squash) September 9, 2023 20:57
@yurishkuro yurishkuro merged commit 81d2cf8 into jaegertracing:main Sep 9, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test TestGetRoundTripperTLSConfig
2 participants